Skip to content

Conversation

@konstntokas
Copy link
Collaborator

@konstntokas konstntokas commented Nov 18, 2025

  • Added subsetting and reprojection in analysis mode via parameters crs
    resolution and bbox.

@codecov
Copy link

codecov bot commented Nov 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
xarray_eopf/amodes/sentinel2.py 100.00% <100.00%> (ø)
xarray_eopf/amodes/sentinel3.py 100.00% <100.00%> (ø)
xarray_eopf/backend.py 100.00% <100.00%> (ø)
xarray_eopf/constants.py 100.00% <100.00%> (ø)
xarray_eopf/utils.py 100.00% <100.00%> (ø)
xarray_eopf/version.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@konstntokas konstntokas requested a review from TonioF November 26, 2025 08:14
Copy link
Contributor

@TonioF TonioF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few minor comments, mostly typos.

Comment on lines +184 to +188
if resolution is None:
if crs is not None and crs.is_geographic:
resolution = 10 / CONVERSION_FACTOR_DEG_METER
else:
resolution = 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have almost exactly the same code in get_native_res. You could simplify this by adapting get_native_res

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_native_res goes from degree to meter, is crs is geographic. res_native = resolution * CONVERSION_FACTOR_DEG_METER Then after that, the nearest equal or coarser Sentinel-2 spatial resolution is selected.

The part above goes from degree to meter to degree, if crs is geographic. resolution = 10 / CONVERSION_FACTOR_DEG_METER Here, no further selection should be done.

Programmatically it would be possible, but logically it think it would complicate things.

crs = pyproj.CRS.from_wkt(ds.spatial_ref.attrs["crs_wkt"])
if bbox is None:
res, ds = next(iter(datasets.items()))
crs_data = pyproj.CRS.from_wkt(ds.spatial_ref.attrs["crs_wkt"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name 'source_crs' would have made it a lot clearer to me what is happening here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change crs_data to source_crs

konstntokas and others added 4 commits November 26, 2025 12:52
@konstntokas konstntokas requested a review from TonioF November 26, 2025 12:08
@konstntokas konstntokas merged commit 1c5a5e3 into main Nov 26, 2025
2 checks passed
@konstntokas konstntokas deleted the konstntokas-xxx-allow_subsetting branch November 26, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants